Skip to content

Extend external LammpsLibrary interface - #237

Merged
srmnitc merged 2 commits into
ICAMS:mainfrom
jan-janssen:clear_no_close
Mar 30, 2026
Merged

srmnitc merged 2 commits into
ICAMS:mainfrom
jan-janssen:clear_no_close

Conversation

@jan-janssen

Copy link
Copy Markdown
Contributor

After some additional testing I noticed that the default setting of closing the LAMMPS library after a given simulation task is confusing when using an external LammpsLibrary interface. So in this pull request I propose to just clear the state of the library rather than closing the library interface completely. Still this comes at the cost that the user has to close the library at the end.

@srmnitc
srmnitc merged commit e591077 into ICAMS:main Mar 30, 2026
4 checks passed
prabhath-c added a commit to prabhath-c/calphy that referenced this pull request Sep 7, 2026
The previous commit tracked session ownership inside LibraryRunner
itself (_owns_lmp, checked in close(), with clear() issued at
construction time). Same outcome, wrong layer: LibraryRunner had to
know something that's really a property of the job, not the runner.

Move the decision to Phase.lammps_close, the method every driver stage
already calls when it's done with its runner for that stage:

    def lammps_close(self, lmp):
        if self.lmp is not None and self.calc.execution_mode == "library":
            lmp.clear()
        else:
            lmp.close()

self.lmp (set once in Phase.__init__, reused across every stage) is
exactly "was an external session ever supplied to this job" -- the
execution_mode check guards the one edge case where it wouldn't apply
(an lmp= passed with execution_mode="executable" is never touched by
create_object, so there is nothing to reuse). LibraryRunner goes back
to being a dumb wrapper: close() always closes, no ownership tracking.
Added clear() as its per-stage counterpart, called from lammps_close
instead of at construction -- the same effect, timed like the actual
end of a stage rather than the start of the next one.

Based on the same pattern from ICAMS#237 (jan-janssen,
"Extend external LammpsLibrary interface"), which solved this exact
problem the same way on the pre-refactor codebase before pylammpsmpi
support was removed and later re-added without carrying this over.

Tests reworked to match: dropped the _owns_lmp-specific ones, added
coverage for LibraryRunner.clear() directly and for lammps_close's
three branches (external+library -> clear, own session -> close,
external+non-library -> close), plus the same multi-stage reuse
regression test, now driven through job.lammps_close(...) instead of
calling LibraryRunner.close() directly.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants